-
Notifications
You must be signed in to change notification settings - Fork 611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[cmd] Add Scheduled command decorator #7083
base: main
Are you sure you want to change the base?
[cmd] Add Scheduled command decorator #7083
Conversation
This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR. |
/format |
…tro2020/allwpilib into ScheduledCommand-decorator
/format |
/format |
/format |
How is this one looking? |
/format |
You have to do this as a seperate overload as the logic internally will be different |
It's the internal logic that I'm struggling with. I've posted the initial setup, but I'm not sure how to join m_ptr to the vector argument. |
…tro2020/allwpilib into ScheduledCommand-decorator
/format |
wpilibNewCommands/src/main/native/cpp/frc2/command/CommandPtr.cpp
Outdated
Show resolved
Hide resolved
I feel like accepting additional commands here messes up the semantics a bit. For the few cases that need a multi-command ScheduleCommand, a static factory (or even the constructor) should serve with more fitting semantics. |
So more like Commands.fork() instead of command.fork() ? |
So make it a static method not an instance method? |
My interpretation of this is that there should be |
Yes, that's what I meant. |
Okay
I gotcha. I'll have to change the robotpy version too. Somehow it's already been merged |
…tro2020/allwpilib into ScheduledCommand-decorator
/format |
/format |
I'm not sure what the cpp error is |
This seemed to be missing as most of the other commands have a decorator.
I'm struggling a bit with the C++